fix(install): exclude .apm-pin marker from package content hash (unblocks v0.12.2 release)#1142
Conversation
Re-installing any package previously installed by APM 0.12.2 erased its apm_modules/ directory: the .apm-pin cache marker (added by #1137 for drift-replay) is written AFTER hash recording, so the on-disk hash diverged from the lockfile hash on every subsequent install, falsely tripping the supply-chain content-hash mismatch check and rmtree'ing the cache. Surfaced by tests/integration/test_guardrailing_hero_e2e.py, which runs 'apm install <virtual-file>' twice via run_command(show_output= True) and asserts apm_modules/github/<flattened-name> still exists -- the v0.12.2 release tag failed in CI on every platform on this test. Fix: exclude .apm-pin from compute_package_hash via a new _EXCLUDED_FILES set, mirroring _EXCLUDED_DIRS. Add regression test test_skips_apm_pin_marker. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in APM's package-integrity hashing so reinstalling an already-cached package does not falsely fail the supply-chain content check and delete the installed package directory.
Changes:
- Excludes
.apm-pinfromcompute_package_hashso cache metadata does not perturb the recorded package hash. - Adds a unit regression test covering the reinstall/hash-stability scenario.
- Adds a changelog entry documenting the reinstall fix for the pending release.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/utils/content_hash.py |
Excludes .apm-pin during package hash computation. |
tests/unit/test_content_hash.py |
Adds regression coverage for .apm-pin hash exclusion. |
CHANGELOG.md |
Documents the reinstall/hash-mismatch fix. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
| if rel.name in _EXCLUDED_FILES: | ||
| continue |
There was a problem hiding this comment.
Good catch -- applied in 9cf5452. Scoped the exclusion to root-level only (len(rel.parts) == 1 and rel.name in _EXCLUDED_ROOT_FILES) and extended the regression test to assert a nested subdir/.apm-pin is still hashed. The cache-pin marker is only ever written by cache_pin.write_marker(install_path, ...) to install_path / MARKER_FILENAME (always at the package root), so this is purely defense-in-depth -- but worth the tightening to keep the marker name from being a future blind spot in the integrity hash.
| ### Fixed | ||
|
|
||
| - **Parallel subdir install race.** `apm install` no longer intermittently fails with `RuntimeError: Subdirectory '<path>' not found in repository` when multiple dependencies (including ADO sub-path deps) resolve to different subdirectories of the same `repo@ref`. The shared clone cache now stores subdir-agnostic bare clones and each consumer materializes its own working tree (mirrors the WS3 `GitCache` pattern). (#1135, fixes #1126, fixes #1140) | ||
| - **Re-installing the same package no longer rmtree's it.** `compute_package_hash` now excludes the `.apm-pin` cache marker (introduced by #1137) so the supply-chain content-hash check sees a stable hash across installs instead of falsely tripping and deleting `apm_modules/<owner>/<pkg>/`. (#1142, regression from #1137) |
There was a problem hiding this comment.
Not applicable in this case. The ## [0.12.2] section was added by the (already merged) release PR #1141, but the v0.12.2 git tag was deleted before any artifact was published -- v0.12.2 has never actually shipped. We're re-cycling the same version number with this fix included, so updating that section in place is correct. Moving the entry under ## [Unreleased] would imply a different version is coming next, which would itself drift from the release that's about to be tagged.
Address Copilot review feedback on PR #1142: a basename match would exclude any .apm-pin file at any depth, so a malicious package could hide bytes under subdir/.apm-pin to bypass the integrity hash. The cache-pin marker is only ever written to the package root by cache_pin.write_marker, so scope the exclusion accordingly. Extend the regression test to assert nested .apm-pin files are still hashed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TL;DR
The v0.12.2 release tag (deleted) failed integration tests on every platform because re-installing any APM-managed package triggered a false supply-chain hash mismatch and
rmtree'd the package directory. Root cause: the.apm-pincache marker added by #1137 was being included incompute_package_hashdespite being written AFTER hash recording. Fix: exclude.apm-pinfrom package hashing.Problem (WHY)
After PR #1137 introduced the
.apm-pindrift-replay cache marker, every secondapm installof an already-installed package observed:…then
safe_rmtree'dapm_modules/<owner>/<pkg>/andsys.exit(1).This blocked the v0.12.2 release --
tests/integration/test_guardrailing_hero_e2e.py::test_2_minute_guardrailing_flowfailed on macOS-x86, macOS-arm, and Linux-arm in run 25369501023. The test callsrun_command(show_output=True)which runs eachapm installtwice (once for display, once for capture) -- triggering the bug deterministically.Approach (WHAT)
compute_package_hashalready excluded.git/and__pycache__/directories. Add a parallel_EXCLUDED_FILES = {".apm-pin"}and skip matching files in the regular-files collection loop.Implementation (HOW)
Why
.apm-pinis correct to exclude: it is install-process metadata (pin marker for drift replay), not package content. The marker is written byLockfileBuilder._sync_cache_pin_markersAFTER_compute_hashruns inFreshDependencySource.acquire, so including it can only make the on-disk hash diverge from the lockfile-recorded hash. Excluding it makes the hash stable across installs.Validation
test_skips_apm_pin_markerintests/unit/test_content_hash.py.test_content_hash.pypass locally.dist/apm-darwin-arm64/apmfrom this branch;pytest tests/integration/test_auto_install_e2e.py tests/integration/test_guardrailing_hero_e2e.py-- 5 passed in 134s, including the previously-failingtest_2_minute_guardrailing_flow.uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/-- silent.Follow-ups (out of scope)
src/apm_cli/install/orsrc/apm_cli/deps/, not just on tag push -- this regression would have been caught at PR feat(audit): default-on integration drift detection #1137 time.FreshDependencySource.acquireswallows exceptions atsources.py:632and exits 0; worth surfacing as a non-zero exit. (Separate issue.)Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com